Make diff section contents red(-)/green(+) in run-tests.php #5965
Conversation
This deliberately uses only one color. Green would be associated with things that were done correctly. But both additional lines and missing lines contribute to the failure, so that may be confusing for something that's an error report instead of a diff that is meant to be applied. Red would make the failure causes stand out visually when scrolling through errors.
run-tests.php
Outdated
| @@ -2885,6 +2886,23 @@ function generate_array_diff(array $ar1, array $ar2, bool $is_reg, array $w): ar | |||
| $old1 = array(); | |||
| $old2 = array(); | |||
|
|
|||
| $format_diff_line = function (int $line_number, string $contents, string $diff_character) use ($colorize): string { | |||
There was a problem hiding this comment.
Can't you move the global $colorize; inside this function body? As it's currently not used anywhere else by generate_array_diff()
There was a problem hiding this comment.
I can, I didn't know how much reviewers would care about efficiency. I'd expect repeatedly looking up the same global on every closure call to make this a bit slower for diffs with a large amount of unexpected actual output, but the closure invocation itself would be a bit slower and this probably doesn't need to be optimized
The closure invocation itself would be much slower than the addition of adding a global $colorize;, though
There was a problem hiding this comment.
tbh I didn't think about performance that much, but I could see it having an impact in that case where having it important once be better.
run-tests.php
Outdated
| $format_expected_line = function (int $line_number, string $contents) use ($format_diff_line): string { | ||
| return $format_diff_line($line_number, $contents, '-'); | ||
| }; | ||
| $format_actual_line = function (int $line_number, string $contents) use ($format_diff_line): string { | ||
| return $format_diff_line($line_number, $contents, '+'); | ||
| }; |
There was a problem hiding this comment.
I think I would prefer that the expected and actual line be different colours, I don't know which ones to choose however. As I personally tend to get confused by which is which >.<
There was a problem hiding this comment.
Light red and dark red? (may be unreadable on screens/terminals with poor contrast with a black background)
Colorizing just the '-' and '+' differently is one other option, but I don't know how readable that would be
There was a problem hiding this comment.
Orange and Light red might be better? But, the more I think about it, the less I know what is a good solution to this problem, so maybe just having the whole diff as one colour is fine.
There was a problem hiding this comment.
If we want to color diffs, let's color them in the usual way (red and green)...
There was a problem hiding this comment.
This deliberately uses one color. Green would be associated with things that were done correctly.
But both additional lines and missing lines contribute to the failure,
so that may be confusing for something that's an error report instead of a diff
that is meant to be applied.
That's what I initially planned to do, but I updated that before committing because it seemed misleading to use green for a line that's an error. Green definitely makes sense for git/github diffs, patch proposals, etc., but seems less clear for an error message
For example, if context was added (-C NUM_LINES, like diff -C 3), then light green would be appropriate for the surrounding lines that actually matched the expected lines (although light grey or no colorizing would be my preference for matching context lines)
There was a problem hiding this comment.
I think you're overthinking this. The diff colors are not a value judgement. Red and green does not mean "bad" and "good", it just means "removed" and "added".
Adding some context to diffs sounds like a good idea.
There was a problem hiding this comment.
Adding some context to diffs sounds like a good idea.
Agreed, I was wondering why that was never supported. I guess the line number of the expected(green) file could be used.
If there are substantial objections to the color, I guess an environment variable, CLI flag, or an RFC with secondary votes on the run-tests.php colorizing style could be created.
I was probably overthinking this out of concern of others objecting - I locally use colordiff for error reporting in another project without issues
There was a problem hiding this comment.
A screenshot of what it will look like
TysonAndre
changed the title
|
The build failure seems unrelated to this PR
|
Mentioned in php#5965 (comment) This PR proposes 3 lines of context so the impact can be seen in tests. Other `diff` programs show around 3 lines of context. (This helps indicate exactly which position a test should be updated to add a new expected line at)
|
I have temporarily reverted this change, as it adds the coloring not just to the |
Mentioned in php#5965 (comment) This PR proposes 3 lines of context so the impact can be seen in tests. Other `diff` programs show around 3 lines of context. (This helps indicate exactly which position a test should be updated to add a new expected line at) Use the mapping for choosing order to display diffs Properly include context in cases where the expected output had more lines than the actual output, e.g. ``` --FILE-- A A1 A C NEARBY --EXPECTF-- A B A1 B A B A B NEARBY ```
Mentioned in php#5965 (comment) This PR proposes 3 lines of context so the impact can be seen in tests. Other `diff` programs show around 3 lines of context. (This helps indicate exactly which position a test should be updated to add a new expected line at) Use the mapping for choosing order to display diffs Properly include context in cases where the expected output had more lines than the actual output, e.g. ``` --FILE-- A A1 A C NEARBY --EXPECTF-- A B A1 B A B A B NEARBY ```
Mentioned in #5965 (comment) This PR proposes 3 lines of context so the impact can be seen in tests. Other `diff` programs show around 3 lines of context. (This helps indicate exactly which position a test should be updated to add a new expected line at) Use the mapping for choosing order to display diffs Properly include context in cases where the expected output had more lines than the actual output, e.g. ``` --FILE-- A A1 A C NEARBY --EXPECTF-- A B A1 B A B A B NEARBY ``` Closes GH-5968
This deliberately uses one color. Green would be associated with things that were done correctly.But both additional lines and missing lines contribute to the failure,
so that may be confusing for something that's an error report instead of a diff
that is meant to be applied.
This uses green for lines with
+and red for lines with-.RedColors(Red and Green) would make the failure causes stand out visually when scrolling through errors.@Girgias @nikic - thoughts?
For example, https://docs.pytest.org/en/stable/getting-started.html seems to make the entire diff section red.
Some other tools like PHPUnit don't seem to colorize diffs at all, just the overall summary.